Update DevicePool to provide multiple devices #1142
Conversation
- Remove mismatched device pool, make the device pool could provide multiple devices at once, and reserve a default mismatched device at test initialization. - Cancel selecting the default mismatched device in tests. For requesting mismatched device with particular features, we still need to call selectMismatchedDeviceOrSkipTestCase.
|
Previews, as seen when this build job started (c51c6a2): |
|
Hi @kainino0x , we'd like to invite you to review this PR as it changes the test framework. Could you take a look? |
| /** Device with no descriptor for device mismatch validation. */ | ||
| private mismatchedDefaultHolder: DeviceHolder | 'uninitialized' | 'failed' = 'uninitialized'; | ||
| /** Devices with descriptors for device mismatch validation. */ | ||
| private mismatchedNonDefaultHolders = new DescriptorToHolderMap(); |
There was a problem hiding this comment.
This looks essentially just like having two DevicePool objects (like before), but they've been merged into one object. What's the benefit to that?
There was a problem hiding this comment.
The reason for adding new properties, rather than expanding the existing holders into an array, is to make it easier to know where to delete the device when releasing a device, otherwise we need to go through all devices in the holders to check if it's default device or mismatched device.
And if change defaultHolder and holders in nonDefaultHolders to array, we use the first solt of DeviceHolders[] as the default device and the second solt as the mismatched device (so I wanted to change the mismatchedDevice to secondaryDevice before), but in terms of code reading, I think it is not very friendly to know their usages.
There was a problem hiding this comment.
I understand why you do it as separate properties rather than an array, it's definitely a simpler change. But why do we need this change at all? It seems functionally identical to just having two DevicePools, which are more cleanly separated from each other.
I think you also made every GPUTest have two devices (primary and mismatched). That seems like it can be done just fine with two DevicePools. That said, I still don't think it's good to have every test in the entire test suite use two devices. Only the device-mismatch tests need them, and I don't think one line of setup in those tests is a problem. (Right now it's three, because of the if (mismatched) check, but that's not really necessary.)
There was a problem hiding this comment.
Oh! Do you meams why remove the these lines:
if (mismatched) {
await t.selectMismatchedDeviceOrSkipTestCase(undefined);
}
I remember that you had a comment at #741, said
Come to think of it, it's also unfortunate that selectMismatchedDeviceOrSkipTestCase needs to be called in every test separately. It would be ideal if it happened implicitly, and if it wasn't necessary to pass in a device descriptor at all - the mismatched device should always have the same device descriptor as the original device
If we want to have a mismatched device implicitly, we need to initialize the device in every test, otherwise selectMismatchedDeviceOrSkipTestCase is needed in mismatched validation tests.
This looks essentially just like having two DevicePool objects (like before), but they've been merged into one object.
I'm not sure if I misunderstood the DevicePool support multiple devices here. What I throught was adding more DeviceHolders in the DevicePool (using array or more properties) or making DeviceHolder could contain more than one devices with same descripters (what I did in previous PR). Isn't this what you mean by supporting multiple devices in DevicePool?
There was a problem hiding this comment.
I remember that you had a comment at #741, said
Come to think of it, it's also unfortunate that selectMismatchedDeviceOrSkipTestCase needs to be called in every test separately. It would be ideal if it happened implicitly, and if it wasn't necessary to pass in a device descriptor at all - the mismatched device should always have the same device descriptor as the original device
Ah yes, I remember that. I think the implementation I had in mind was, if a test uses the .mismatchedDevice property (or .getMismatchedDevice()) then we would get one automatically, rather than having to request one. I don't think we should do it for all tests regardless of whether they need one.
I'm not sure if I misunderstood the DevicePool support multiple devices here.
The intent of this MAINTENANCE_TODO was to generalize the DevicePool so we can run multiple tests in parallel (since tests are async). But it hasn't been important so we haven't done anything about it.
| // Always attempt to initialize default device, to see if it succeeds. | ||
| let errorMessage = ''; | ||
| if (this.defaultHolder === 'uninitialized') { | ||
| let defaultHolder = mismatched ? this.mismatchedDefaultHolder : this.defaultHolder; |
There was a problem hiding this comment.
nit: this isn't "default holder", should just be called holder
multiple devices at once, and reserve a default mismatched device
at test initialization.
requesting mismatched device with particular features, we still need
to call selectMismatchedDeviceOrSkipTestCase.
Issue: #912
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.